[amplipi] Initial contribution of AmpliPi binding #10983

kaikreuzer · 2021-07-12T19:48:29Z

This binding supports the AmpliPi home audio system - see https://www.micro-nova.com/amplipi.

Signed-off-by: Kai Kreuzer [email protected]

sumnerboy12 · 2021-07-13T06:37:52Z

Nice, following with interest! Once I get mine I might be able to help out also.

kaikreuzer · 2021-07-17T07:16:57Z

FTR, some discussion on the right modelling is taking place at micro-nova/AmpliPi#54.

Signed-off-by: Kai Kreuzer <[email protected]>

kaikreuzer · 2021-07-28T08:37:22Z

@openhab/add-ons-maintainers This is now ready for review. The implementation contains the main features that make sense to have. Future PRs will add further functionality.

bundles/org.openhab.binding.amplipi/README.md

fwolter

Did you add the generated files by intention?

There are some checkstyle warnings left.

fwolter · 2021-07-31T16:33:58Z

....binding.amplipi/src/main/java/org/openhab/binding/amplipi/internal/AmpliPiGroupHandler.java

+        super(thing);
+        this.httpClient = httpClient;
+        this.gson = new Gson();
+        id = Integer.valueOf(thing.getConfiguration().get(AmpliPiBindingConstants.CFG_PARAM_ID).toString());


Shouldn't this be set in initialize() to be updated on configuration updates?

Good point. I have now removed the field completely and use a method instead. This way, we can be sure that it's always the latest value that is used.

fwolter · 2021-07-31T16:35:44Z

....binding.amplipi/src/main/java/org/openhab/binding/amplipi/internal/AmpliPiGroupHandler.java

+                updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR,
+                        "AmpliPi request failed: " + e.getMessage());
+            } catch (ExecutionException e) {
+                logger.error("HTTP request to AmpliPi failed: {}.", e.getMessage());


It doesn't seem resonable to log to error when the network fails.

If there's a network issue, we run into the TimeoutException which is caught above.
I am not exactly sure in which situations an ExecutionException exactly occurs - that's why I rather considered it a bug situation as long as it isn't better handled. Do you happen to know when we can expect the ExecutionException from the httpClient?

Interesting question. I always assumed that a ExecutionException can be caused by a network failure. I did a quick search and couldn't find any official documentation, but I found some stack traces showing this (in an OH context):

Caused by: java.util.concurrent.ExecutionException: java.net.ConnectException: Connection refused at org.eclipse.jetty.client.util.FutureResponseListener.getResult(FutureResponseListener.java:118) ...

I would handle this as a network failure.

I am not sure. A "Connection refused" means that the network is available, but that the contacted device did not accept the request at all - meaning that the request was invalid (e.g. by using a wrong port), so it might be due to a bug in the code.
But it's ok for me to treat it like a network issue - users will complain, if their things go OFFLINE for an ununderstood reason anyhow. I've changed the code accordingly.

Or the host is available, but the service is down. I think you can't clearly differentiate between temprorary or fatal errors here.

...enhab.binding.amplipi/src/main/java/org/openhab/binding/amplipi/internal/AmpliPiHandler.java

...b.binding.amplipi/src/main/java/org/openhab/binding/amplipi/internal/AmpliPiZoneHandler.java

Signed-off-by: Kai Kreuzer <[email protected]>

kaikreuzer · 2021-07-31T20:41:21Z

Did you add the generated files by intention?

Yes, absolutely. The code has actually be manually adapted after generation and the generator is configured to not overwrite existing files. I thought I leave the maven plugin in the pom, so that it is easier in future to generate additional model classes when the Swagger file is extended by new features.

Signed-off-by: Kai Kreuzer <[email protected]>

fwolter

LGTM

fwolter · 2021-08-01T18:23:58Z

The build leaves an untracked file: bundles/org.openhab.binding.amplipi/.openapi-generator/amplipi-api.yml.sha256 Should we add it to gitignore?

kaikreuzer · 2021-08-02T07:10:45Z

Yes. I have created #11081 for it.

* Initial contribution of AmpliPi binding Signed-off-by: Kai Kreuzer <[email protected]> * change http client from cxf jax-rs to Jetty Signed-off-by: Kai Kreuzer <[email protected]> * applied spotless Signed-off-by: Kai Kreuzer <[email protected]> * Remove Jackson dependency Signed-off-by: Kai Kreuzer <[email protected]> * Add support for input handling Signed-off-by: Kai Kreuzer <[email protected]> * Clean up, improvements and documentation Signed-off-by: Kai Kreuzer <[email protected]> * Remove unused password from configuration class Signed-off-by: Kai Kreuzer <[email protected]> * Remove example properties file Signed-off-by: Kai Kreuzer <[email protected]> * revert change in .gitignore Signed-off-by: Kai Kreuzer <[email protected]> * Update README Signed-off-by: Kai Kreuzer <[email protected]> * Address review feedback Signed-off-by: Kai Kreuzer <[email protected]> * Handle ExecutionException as network error Signed-off-by: Kai Kreuzer <[email protected]>

* Initial contribution of AmpliPi binding Signed-off-by: Kai Kreuzer <[email protected]> * change http client from cxf jax-rs to Jetty Signed-off-by: Kai Kreuzer <[email protected]> * applied spotless Signed-off-by: Kai Kreuzer <[email protected]> * Remove Jackson dependency Signed-off-by: Kai Kreuzer <[email protected]> * Add support for input handling Signed-off-by: Kai Kreuzer <[email protected]> * Clean up, improvements and documentation Signed-off-by: Kai Kreuzer <[email protected]> * Remove unused password from configuration class Signed-off-by: Kai Kreuzer <[email protected]> * Remove example properties file Signed-off-by: Kai Kreuzer <[email protected]> * revert change in .gitignore Signed-off-by: Kai Kreuzer <[email protected]> * Update README Signed-off-by: Kai Kreuzer <[email protected]> * Address review feedback Signed-off-by: Kai Kreuzer <[email protected]> * Handle ExecutionException as network error Signed-off-by: Kai Kreuzer <[email protected]> Signed-off-by: Dave J Schoepel <[email protected]>

* Initial contribution of AmpliPi binding Signed-off-by: Kai Kreuzer <[email protected]> * change http client from cxf jax-rs to Jetty Signed-off-by: Kai Kreuzer <[email protected]> * applied spotless Signed-off-by: Kai Kreuzer <[email protected]> * Remove Jackson dependency Signed-off-by: Kai Kreuzer <[email protected]> * Add support for input handling Signed-off-by: Kai Kreuzer <[email protected]> * Clean up, improvements and documentation Signed-off-by: Kai Kreuzer <[email protected]> * Remove unused password from configuration class Signed-off-by: Kai Kreuzer <[email protected]> * Remove example properties file Signed-off-by: Kai Kreuzer <[email protected]> * revert change in .gitignore Signed-off-by: Kai Kreuzer <[email protected]> * Update README Signed-off-by: Kai Kreuzer <[email protected]> * Address review feedback Signed-off-by: Kai Kreuzer <[email protected]> * Handle ExecutionException as network error Signed-off-by: Kai Kreuzer <[email protected]>

kaikreuzer added the new binding If someone has started to work on a binding. For a new binding PR. label Jul 12, 2021

kaikreuzer mentioned this pull request Jul 12, 2021

Feature Request: Add openHAB Integration/bindings micro-nova/AmpliPi#54

Closed

Skinah changed the title ~~Initial contribution of AmpliPi binding~~ [amplipi] Initial contribution of AmpliPi binding Jul 13, 2021

kaikreuzer force-pushed the amplipi branch from 193372c to df3dfff Compare July 13, 2021 12:10

kaikreuzer added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jul 15, 2021

Initial contribution of AmpliPi binding

a89e13c

Signed-off-by: Kai Kreuzer <[email protected]>

kaikreuzer force-pushed the amplipi branch from df3dfff to a89e13c Compare July 18, 2021 08:26

kaikreuzer added 3 commits July 22, 2021 22:31

change http client from cxf jax-rs to Jetty

26ce7ba

Signed-off-by: Kai Kreuzer <[email protected]>

applied spotless

b19c6cb

Signed-off-by: Kai Kreuzer <[email protected]>

Remove Jackson dependency

bfaa3ba

Signed-off-by: Kai Kreuzer <[email protected]>

kaikreuzer added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jul 27, 2021

kaikreuzer added 6 commits July 27, 2021 19:45

Add support for input handling

dc6aa2f

Signed-off-by: Kai Kreuzer <[email protected]>

Clean up, improvements and documentation

0410dd8

Signed-off-by: Kai Kreuzer <[email protected]>

Remove unused password from configuration class

b0d718d

Signed-off-by: Kai Kreuzer <[email protected]>

Remove example properties file

a5c8541

Signed-off-by: Kai Kreuzer <[email protected]>

revert change in .gitignore

9610855

Signed-off-by: Kai Kreuzer <[email protected]>

Update README

785acba

Signed-off-by: Kai Kreuzer <[email protected]>

kaikreuzer added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jul 28, 2021

kaikreuzer marked this pull request as ready for review July 28, 2021 08:36

kaikreuzer requested a review from a team as a code owner July 28, 2021 08:36

sumnerboy12 reviewed Jul 29, 2021

View reviewed changes

bundles/org.openhab.binding.amplipi/README.md Outdated Show resolved Hide resolved

fwolter requested changes Jul 31, 2021

View reviewed changes

Address review feedback

ab77e54

Signed-off-by: Kai Kreuzer <[email protected]>

kaikreuzer added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jul 31, 2021

Handle ExecutionException as network error

5a227e1

Signed-off-by: Kai Kreuzer <[email protected]>

fwolter approved these changes Aug 1, 2021

View reviewed changes

fwolter merged commit f25cc8d into openhab:main Aug 1, 2021

fwolter added this to the 3.2 milestone Aug 1, 2021

kaikreuzer deleted the amplipi branch August 1, 2021 10:32

kaikreuzer mentioned this pull request Aug 2, 2021

[amplipi] Ignore files from openapi-generator #11081

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[amplipi] Initial contribution of AmpliPi binding #10983

[amplipi] Initial contribution of AmpliPi binding #10983

kaikreuzer commented Jul 12, 2021 •

edited

Loading

sumnerboy12 commented Jul 13, 2021

kaikreuzer commented Jul 17, 2021

kaikreuzer commented Jul 28, 2021

fwolter left a comment

fwolter Jul 31, 2021

kaikreuzer Jul 31, 2021

fwolter Jul 31, 2021

kaikreuzer Jul 31, 2021

fwolter Aug 1, 2021

kaikreuzer Aug 1, 2021

fwolter Aug 1, 2021

kaikreuzer commented Jul 31, 2021

fwolter left a comment

fwolter commented Aug 1, 2021

kaikreuzer commented Aug 2, 2021

[amplipi] Initial contribution of AmpliPi binding #10983

[amplipi] Initial contribution of AmpliPi binding #10983

Conversation

kaikreuzer commented Jul 12, 2021 • edited Loading

sumnerboy12 commented Jul 13, 2021

kaikreuzer commented Jul 17, 2021

kaikreuzer commented Jul 28, 2021

fwolter left a comment

Choose a reason for hiding this comment

fwolter Jul 31, 2021

Choose a reason for hiding this comment

kaikreuzer Jul 31, 2021

Choose a reason for hiding this comment

fwolter Jul 31, 2021

Choose a reason for hiding this comment

kaikreuzer Jul 31, 2021

Choose a reason for hiding this comment

fwolter Aug 1, 2021

Choose a reason for hiding this comment

kaikreuzer Aug 1, 2021

Choose a reason for hiding this comment

fwolter Aug 1, 2021

Choose a reason for hiding this comment

kaikreuzer commented Jul 31, 2021

fwolter left a comment

Choose a reason for hiding this comment

fwolter commented Aug 1, 2021

kaikreuzer commented Aug 2, 2021

kaikreuzer commented Jul 12, 2021 •

edited

Loading